-
Notifications
You must be signed in to change notification settings - Fork 284
Added gencerts command, fixed unreachable code, added missing argument, fixed few golint errors #944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the suggestions below, I'd like to see something testing that the behavior is working as intended. A test might have exposed the x.Testnet
initialization problem I mentioned below.
cmd/setapicreds.go
Outdated
@@ -86,7 +88,7 @@ func (x *SetAPICreds) Execute(args []string) error { | |||
apiCfg.AllowedIPs = []string{} | |||
} | |||
|
|||
if err := r.SetConfigKey("JSON-API", apiCfg); err != nil { | |||
if r.SetConfigKey("JSON-API", apiCfg); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look right. Which err
did you mean to check here?
I'm thinking the linter told you that you can't declare an err
which has already been declared. But when you remove the :
it tells you that err
has not be declared inside the block. If this is the case, I've solved this by declaring the err
at the top and using =
throughout.
cmd/gencerts.go
Outdated
func pemBlockForKey(priv interface{}) *pem.Block { | ||
switch k := priv.(type) { | ||
case *rsa.PrivateKey: | ||
return &pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(k)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preamble "RSA PRIVATE KEY"
should probably be a constant.
cmd/gencerts.go
Outdated
repoPath = x.DataDir | ||
} | ||
|
||
flag.Parse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this being called early enough? Seems x.Testnet
would always be false
here.
cmd/gencerts.go
Outdated
} | ||
|
||
template.IsCA = true | ||
template.KeyUsage |= x509.KeyUsageCertSign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these floating down here like this? Why not add them to the template
declaration on L89?
cmd/gencerts.go
Outdated
} else { | ||
template.DNSNames = append(template.DNSNames, h) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build your []string
for IPAddresses and DNSNames first, then assign it to the struct on L89 all at once.
cmd/restore.go
Outdated
@@ -47,6 +47,7 @@ import ( | |||
"time" | |||
) | |||
|
|||
//Restore struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding empty definitions merely to satisfy the linter defeats the purpose of the linter. Let's remove these or replace them w idiomatic go docs. https://blog.golang.org/godoc-documenting-go-code
@@ -402,7 +402,6 @@ func (n *OpenBazaarNode) SendDisputeClose(peerId string, k *libp2p.PubKey, resol | |||
Payload: a, | |||
} | |||
return n.sendMessage(peerId, k, m) | |||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@@ -388,7 +388,7 @@ func (m *MessageRetriever) handleMessage(env pb.Envelope, addr string, id *peer. | |||
log.Errorf("Error saving offline message %s to database: %s", addr, err.Error()) | |||
} | |||
} else { | |||
log.Errorf("Error serializing offline message %s for storage") | |||
log.Errorf("Error serializing offline message %s for storage", addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Cleanup comments
Nice work @trigun117! Your pull request has been merged and the bounty of |
@chaintip fixes #868